-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store/update credential record last in RP ops #2215
base: main
Are you sure you want to change the base?
Conversation
the [=[RP]=] SHOULD fail the [=registration ceremony=]. | ||
1. If all the above steps are successful, | ||
store |credentialRecord| in the [=user account=] that was denoted in <code>|pkOptions|.{{PublicKeyCredentialCreationOptions/user}}</code> | ||
and continue the [=registration ceremony=] as appropriate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "continue the registration ceremony" mean? The way I interpret that is that RPs may have specific criteria in addition to what's stated in the spec, and this line means for the RP to continue with said criteria. However if that is true, then it's possible the ceremony fails; but we already stated to store the credentialRecord. If my interpretation is correct, then this needs to be re-worded like:
If all the above steps are successful, continue the registration ceremony as appropriate. If the ceremony is successful, then store credentialRecord …
Ditto for the authentication ceremony section.
In other words the last "portion"/clause of the last sentence of the last step should be about storing/updating the credential unless subsequent clauses or sentences do not describe processes that even have the possibility for causing the ceremony to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with these changes.
2024-11-27: @emlun to resolve @zacknewman's comments, then OK to merge. |
Closes #2204.
Preview | Diff